Add list_file() functional API to FSSpecFileLister and IoPathFileLister #463

bushshrub · 2022-05-25T15:19:11Z

Fixes #387

Changes

Adds list_file() method on IoPathFileListerIterDataPipe
Adds list_file() method on FSSpecFileListerIterDataPipe
Add tests for those methods

Additional comments

I feel as if the implementation is quite naive. Would appreciate any feedback on it.

facebook-github-bot · 2022-05-25T15:19:15Z

Hi @xiurobert!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

bushshrub · 2022-05-25T15:28:01Z

Additional notes: Perhaps return list(self) could work as well.

ejguan

Sorry, the functional API refers to functional_datapipe. See ref:

data/torchdata/datapipes/iter/load/fsspec.py

Line 104 in ec83d11

@functional_datapipe("open_file_by_fsspec")

By adding this decorator the class, we are able invoke such class using functional call.

@functional_datapipe("list_file_by_fsspec")
class FSSpecFileListerIterDataPipe(IterDataPipe[str]):
    ...

dp = IterableWrapper(["file://folder", ])
dp = dp.list_file_by_fsspec()  # Functional API here 
list(dp)  # return list of files in folder

bushshrub · 2022-05-25T15:36:27Z

Sorry, my bad. Must have misunderstood. Will make the changes

bushshrub · 2022-05-25T15:49:01Z

@ejguan updated it along with relevant tests

test/test_local_io.py

test/test_fsspec.py

facebook-github-bot · 2022-05-25T16:05:47Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

bushshrub · 2022-05-25T16:08:47Z

All tests are passing on my machine so far.

ejguan

Thank you so much. Overall, LGTM with two minor comments.

test/test_fsspec.py

test/test_local_io.py

NivekT

Thanks for working on this!

Question: Do you plan on working on pytorch/pytorch#78263 as well? I see an issue but not a PR for PyTorch Core.

bushshrub · 2022-05-26T01:21:59Z

Yep, I do intend to do so. I opened the issue because the CONTRIBUTING.md file specifies I should be opening one before working on any "features"

bushshrub · 2022-05-27T03:44:53Z

@NivekT is it ok if I directly open the pull request with PyTorch core? Their contributing guidelines mention that I should be opening an issue before implementing a feature.

bushshrub · 2022-05-27T10:18:08Z

Thanks for working on this!

Question: Do you plan on working on pytorch/pytorch#78263 as well? I see an issue but not a PR for PyTorch Core.

Just saw the PyTorch FAQ, I could probably open the PR since it's a small change.

facebook-github-bot · 2022-05-31T13:41:31Z

@ejguan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

torchdata/datapipes/iter/load/iopath.py

torchdata/datapipes/iter/load/fsspec.py

ejguan

Thank you, LGTM

bushshrub · 2022-05-31T15:34:18Z

Will the APIs in torchdata be updated with the new grammar as well?

ejguan · 2022-05-31T15:35:30Z

Will the APIs in torchdata be updated with the new grammar as well?

Yeah. @NivekT Just opened a PR to change the names of DataPipe. See: #479

bushshrub · 2022-05-31T15:36:21Z

Awesome.

test/test_fsspec.py

ejguan

There are a few bugs in your tests.

test/test_local_io.py

test/test_fsspec.py

ejguan

Could you please also add tests for list_files_by_iopath in

data/test/test_local_io.py

Lines 655 to 673 in aab67f3

    
           @skipIfNoIoPath 
        
           def test_io_path_file_lister_iterdatapipe(self): 
        
               datapipe = IoPathFileLister(root=self.temp_sub_dir.name) 
        
               # check all file paths within sub_folder are listed 
        
               for path in datapipe: 
        
                   self.assertTrue(path in self.temp_sub_files) 
        
           @skipIfNoIoPath 
        
           def test_io_path_file_lister_iterdatapipe_with_list(self): 
        
               datapipe = IoPathFileLister(root=[self.temp_sub_dir.name, self.temp_sub_dir_2.name]) 
        
               file_lister = list(datapipe) 
        
               file_lister.sort() 
        
               all_temp_files = list(self.temp_sub_files + self.temp_sub_files_2) 
        
               all_temp_files.sort() 
        
               # check all file paths within sub_folder are listed 
        
               self.assertEqual(file_lister, all_temp_files)

?

ejguan · 2022-06-01T14:02:56Z

test/test_fsspec.py

+        datapipe = IterableWrapper(["file://" + self.temp_sub_dir.name, "file://" + self.temp_sub_dir_2.name])
+        datapipe = datapipe.list_files_by_fsspec()
+        res = list(datapipe).sort()
+        self.assertEqual(res, temp_files)


It seems the test is still failing here. Could you please fix it by mimicking the test case from line 80 to line 89?

bushshrub · 2022-06-02T01:46:55Z

Could you please also add tests for list_files_by_iopath in

data/test/test_local_io.py

Lines 655 to 673 in aab67f3

@skipIfNoIoPath

def test_io_path_file_lister_iterdatapipe(self):

datapipe = IoPathFileLister(root=self.temp_sub_dir.name)

# check all file paths within sub_folder are listed

for path in datapipe:

self.assertTrue(path in self.temp_sub_files)

@skipIfNoIoPath

def test_io_path_file_lister_iterdatapipe_with_list(self):

datapipe = IoPathFileLister(root=[self.temp_sub_dir.name, self.temp_sub_dir_2.name])

file_lister = list(datapipe)

file_lister.sort()

all_temp_files = list(self.temp_sub_files + self.temp_sub_files_2)

all_temp_files.sort()

# check all file paths within sub_folder are listed

self.assertEqual(file_lister, all_temp_files)

?

I believe they are here https://github.com/pytorch/data/pull/463/files#diff-6e69ca11dfe73793a94592ec3e4a303e6807afa8a7fed4d88168b50d9be829e3R663-R667

ejguan · 2022-06-02T13:20:21Z

You can run your test on your local machine via python test/test_fsspec.py and python test/test_local_io.py. You might need to install fsspec and iopath via pip install fsspec iopath.

bushshrub · 2022-06-02T14:34:18Z

Changes made @ejguan

facebook-github-bot · 2022-06-02T15:19:08Z

@ejguan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ejguan

Thank you, LGTM

Co-authored-by: Kevin Tse <NivekT@users.noreply.github.com>

Co-authored-by: Erjia Guan <68879799+ejguan@users.noreply.github.com>

I completely forgot that sort() was in place and returns None, oops

bushshrub · 2022-06-02T16:18:23Z

Did this work? I squashed the commits locally and force-pushed.

ejguan · 2022-06-02T16:19:17Z

Did this work?

It works now. Let me import your PR. Don't worry about squashing the commits. We will do it automatically.

facebook-github-bot · 2022-06-02T16:19:31Z

@ejguan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

bushshrub · 2022-06-02T16:19:38Z

Great.

…er (pytorch#463) Summary: Fixes pytorch#387 ### Changes - Adds `list_file()` method on `IoPathFileListerIterDataPipe` - Adds `list_file()` method on `FSSpecFileListerIterDataPipe` - Add tests for those methods #### Additional comments I feel as if the implementation is quite naive. Would appreciate any feedback on it. Pull Request resolved: pytorch#463 Reviewed By: NivekT Differential Revision: D36777142 Pulled By: ejguan fbshipit-source-id: 1c4474776f3fcd377ae545bd8bd7bf26d0b2fa88

…er (#463) Summary: Fixes #387 ### Changes - Adds `list_file()` method on `IoPathFileListerIterDataPipe` - Adds `list_file()` method on `FSSpecFileListerIterDataPipe` - Add tests for those methods #### Additional comments I feel as if the implementation is quite naive. Would appreciate any feedback on it. Pull Request resolved: #463 Reviewed By: NivekT Differential Revision: D36777142 Pulled By: ejguan fbshipit-source-id: 1c4474776f3fcd377ae545bd8bd7bf26d0b2fa88

ejguan requested changes May 25, 2022

View reviewed changes

ejguan reviewed May 25, 2022

View reviewed changes

test/test_local_io.py Outdated Show resolved Hide resolved

test/test_local_io.py Outdated Show resolved Hide resolved

test/test_fsspec.py Outdated Show resolved Hide resolved

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 25, 2022

ejguan approved these changes May 25, 2022

View reviewed changes

test/test_fsspec.py Outdated Show resolved Hide resolved

test/test_local_io.py Outdated Show resolved Hide resolved

NivekT approved these changes May 25, 2022

View reviewed changes

NivekT suggested changes May 31, 2022

View reviewed changes

torchdata/datapipes/iter/load/iopath.py Outdated Show resolved Hide resolved

torchdata/datapipes/iter/load/fsspec.py Outdated Show resolved Hide resolved

ejguan approved these changes May 31, 2022

View reviewed changes

bushshrub requested a review from NivekT May 31, 2022 15:33

ejguan reviewed May 31, 2022

View reviewed changes

test/test_fsspec.py Outdated Show resolved Hide resolved

ejguan requested changes May 31, 2022

View reviewed changes

test/test_local_io.py Outdated Show resolved Hide resolved

test/test_fsspec.py Outdated Show resolved Hide resolved

test/test_fsspec.py Outdated Show resolved Hide resolved

bushshrub requested a review from ejguan June 1, 2022 04:02

ejguan reviewed Jun 1, 2022

View reviewed changes

ejguan approved these changes Jun 2, 2022

View reviewed changes

bushshrub and others added 20 commits June 2, 2022 16:17

test: consistency with test_fsspec_file_lister_iterdatapipe_with_list

225189a

test: ensure protocol is present

8da9374

feat: naive impl of list_file for iopath

ab91524

test: test for list_file() of iopath

aafaa71

test: consistency for iopath

774a6ac

feat: functional_datapipe list_file_by

2c52f9d

test: update tests for fsspec and iopath for functional api

44909dd

test: merge functional API tests with regular tests

e777d26

test: delete unnecessary test

04b17d0

style: change variable names for functional lister tests

4d88bed

chore: directly iterate over datapipe

0971a6e

refactor: fix grammar

0155dc1

Co-authored-by: Kevin Tse <NivekT@users.noreply.github.com>

refactor: fix grammar

7c89201

Co-authored-by: Kevin Tse <NivekT@users.noreply.github.com>

test: update tests with new functional API

492c9b3

Update test/test_fsspec.py

626385a

Co-authored-by: Erjia Guan <68879799+ejguan@users.noreply.github.com>

test: fix failing tests due to missing file:// prefix

6e3c661

test: fix breaking test due to unsortable datapipe

3d1702e

test: fix failing test_fsspec_file_lister_iterdp_with_lsit

cf1085c

test: fix failing tests due to .sort() being in-place

2a4efb4

I completely forgot that sort() was in place and returns None, oops

test: fix failing tests due to missing file:// prefix again

ff13537

ejguan force-pushed the 387-filelister-listfile branch from 0726935 to ff13537 Compare June 2, 2022 16:18

facebook-github-bot closed this in f54b3ae Jun 3, 2022

ejguan mentioned this pull request Jun 6, 2022

Add list_file() functional API to FSSpecFileLister and IoPathFileLister (#463) #496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add list_file() functional API to FSSpecFileLister and IoPathFileLister #463

Add list_file() functional API to FSSpecFileLister and IoPathFileLister #463

bushshrub commented May 25, 2022 •

edited

Loading

facebook-github-bot commented May 25, 2022

bushshrub commented May 25, 2022

ejguan left a comment

bushshrub commented May 25, 2022

bushshrub commented May 25, 2022

facebook-github-bot commented May 25, 2022

bushshrub commented May 25, 2022

ejguan left a comment

NivekT left a comment

bushshrub commented May 26, 2022

bushshrub commented May 27, 2022

bushshrub commented May 27, 2022

facebook-github-bot commented May 31, 2022

ejguan left a comment

bushshrub commented May 31, 2022

ejguan commented May 31, 2022

bushshrub commented May 31, 2022

ejguan left a comment

ejguan left a comment

ejguan Jun 1, 2022

bushshrub commented Jun 2, 2022

ejguan commented Jun 2, 2022

bushshrub commented Jun 2, 2022

facebook-github-bot commented Jun 2, 2022

ejguan left a comment

bushshrub commented Jun 2, 2022 •

edited

Loading

ejguan commented Jun 2, 2022

facebook-github-bot commented Jun 2, 2022

bushshrub commented Jun 2, 2022

	@skipIfNoIoPath
	def test_io_path_file_lister_iterdatapipe(self):
	datapipe = IoPathFileLister(root=self.temp_sub_dir.name)

	# check all file paths within sub_folder are listed
	for path in datapipe:
	self.assertTrue(path in self.temp_sub_files)

	@skipIfNoIoPath
	def test_io_path_file_lister_iterdatapipe_with_list(self):
	datapipe = IoPathFileLister(root=[self.temp_sub_dir.name, self.temp_sub_dir_2.name])

	file_lister = list(datapipe)
	file_lister.sort()
	all_temp_files = list(self.temp_sub_files + self.temp_sub_files_2)
	all_temp_files.sort()

	# check all file paths within sub_folder are listed
	self.assertEqual(file_lister, all_temp_files)

Add list_file() functional API to FSSpecFileLister and IoPathFileLister #463

Add list_file() functional API to FSSpecFileLister and IoPathFileLister #463

Conversation

bushshrub commented May 25, 2022 • edited Loading

Changes

Additional comments

facebook-github-bot commented May 25, 2022

Action Required

Process

bushshrub commented May 25, 2022

ejguan left a comment

Choose a reason for hiding this comment

bushshrub commented May 25, 2022

bushshrub commented May 25, 2022

facebook-github-bot commented May 25, 2022

bushshrub commented May 25, 2022

ejguan left a comment

Choose a reason for hiding this comment

NivekT left a comment

Choose a reason for hiding this comment

bushshrub commented May 26, 2022

bushshrub commented May 27, 2022

bushshrub commented May 27, 2022

facebook-github-bot commented May 31, 2022

ejguan left a comment

Choose a reason for hiding this comment

bushshrub commented May 31, 2022

ejguan commented May 31, 2022

bushshrub commented May 31, 2022

ejguan left a comment

Choose a reason for hiding this comment

ejguan left a comment

Choose a reason for hiding this comment

ejguan Jun 1, 2022

Choose a reason for hiding this comment

bushshrub commented Jun 2, 2022

ejguan commented Jun 2, 2022

bushshrub commented Jun 2, 2022

facebook-github-bot commented Jun 2, 2022

ejguan left a comment

Choose a reason for hiding this comment

bushshrub commented Jun 2, 2022 • edited Loading

ejguan commented Jun 2, 2022

facebook-github-bot commented Jun 2, 2022

bushshrub commented Jun 2, 2022

bushshrub commented May 25, 2022 •

edited

Loading

bushshrub commented Jun 2, 2022 •

edited

Loading